Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

operator: add zone aware API spec validation #9378

Merged
merged 31 commits into from
May 17, 2023

Conversation

aminesnow
Copy link
Contributor

@aminesnow aminesnow commented May 3, 2023

What this PR does / why we need it:
Following #9315, this PR adds validation to the zones awareness spec of the LokiStack API.

Special notes for your reviewer:
@periklis

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Documentation added
  • Tests updated
  • CHANGELOG.md updated
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/upgrading/_index.md

@aminesnow aminesnow requested a review from a team as a code owner May 3, 2023 09:58
Mohamed-Amine Bouqsimi added 2 commits May 3, 2023 11:59
Copy link
Collaborator

@xperimental xperimental left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, I don't know if this validation is a good idea, because it might hinder the admin to make changes to a malfunctioning cluster as, depending on the failure, the list of nodes might not contain all the "normally available values" then.

In my opinion, the validation should only fail on configurations which "will absolutely never work" and not depend on the current state.

operator/internal/validation/lokistack.go Outdated Show resolved Hide resolved
Mohamed-Amine Bouqsimi and others added 14 commits May 4, 2023 18:01
Continuation of grafana#9370

Signed-off-by: Danny Kopping <danny.kopping@grafana.com>
Signed-off-by: Danny Kopping <danny.kopping@grafana.com>
**What this PR does / why we need it**:

Update docs and changelog with Loki 2.8.2 changes.

**Which issue(s) this PR fixes**:
Fixes #<issue number>

**Special notes for your reviewer**:

**Checklist**
- [ ] Reviewed the
[`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md)
guide (**required**)
- [ ] Documentation added
- [ ] Tests updated
- [ ] `CHANGELOG.md` updated
- [ ] Changes that require user attention or interaction to upgrade are
documented in `docs/sources/upgrading/_index.md`
Signed-off-by: Joao Marcal <jmarcal@redhat.com>
…r) (grafana#9385)

**Here is a summary of the updates contained in this PR:**
***
Update attribute `$.appVersion` in yaml file
`./production/helm/loki/Chart.yaml` to the following value: `2.8.2`
***
Bump version of Helm Chart
Add changelog entry to `./production/helm/loki/CHANGELOG.md`
Re-generate docs

Co-authored-by: Salva Corts <salva.corts@grafana.com>
…er) (grafana#9388)

**Here is a summary of the updates contained in this PR:**
***
Update attribute `$.enterprise.version` in yaml file
`./production/helm/loki/values.yaml` to the following value: `v1.7.2`
***
Bump version of Helm Chart
Add changelog entry to `./production/helm/loki/CHANGELOG.md`
Re-generate docs

---------

Co-authored-by: Salva Corts <salva.corts@grafana.com>
**What this PR does / why we need it**:

Part of v2.8.2 release

---------

Co-authored-by: J Stickler <julie.stickler@grafana.com>
**What this PR does / why we need it**:

**Which issue(s) this PR fixes**:
Fixes #<issue number>

**Special notes for your reviewer**:

**Checklist**
- [ ] Reviewed the
[`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md)
guide (**required**)
- [ ] Documentation added
- [ ] Tests updated
- [ ] `CHANGELOG.md` updated
- [ ] Changes that require user attention or interaction to upgrade are
documented in `docs/sources/upgrading/_index.md`
…na#9362)

**What this PR does / why we need it**:
Fixes an error on the ServiceMonitor generated when
```
memberlist_ring_enabled: true,
create_service_monitor: true,
```
It also adds some checks on `memberlist.libsonnet` to avoid generating
invalid kubernetes manifests.

**Which issue(s) this PR fixes**:
Fixes grafana#9361
compactor now also supports tsdb in addition to boltdb-schipper

**What this PR does / why we need it**:

**Which issue(s) this PR fixes**:
Fixes #<issue number>

**Special notes for your reviewer**:

**Checklist**
- [ ] Reviewed the
[`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md)
guide (**required**)
- [ ] Documentation added
- [ ] Tests updated
- [ ] `CHANGELOG.md` updated
- [ ] Changes that require user attention or interaction to upgrade are
documented in `docs/sources/upgrading/_index.md`
@aminesnow aminesnow requested a review from JStickler as a code owner May 4, 2023 16:27
@pull-request-size pull-request-size bot added size/XL and removed size/L labels May 4, 2023
@github-actions github-actions bot added area/helm type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories labels May 4, 2023
@pull-request-size pull-request-size bot added size/L and removed size/XL labels May 4, 2023
@github-actions github-actions bot removed type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories area/helm labels May 4, 2023
Copy link
Collaborator

@periklis periklis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After digging a bit into the k8s docs, I suggest to take a different course of action here, since PodTopologyConstraints solving the issue for node labels internally offering the configuration option minDomains (See https://kubernetes.io/docs/concepts/scheduling-eviction/topology-spread-constraints/#spread-constraint-definition):

minDomains indicates a minimum number of eligible domains. This field is optional. A domain is a particular instance of a topology. An eligible domain is a domain whose nodes match the node selector.

WDYT?

@aminesnow
Copy link
Contributor Author

After digging a bit into the k8s docs, I suggest to take a different course of action here, since PodTopologyConstraints solving the issue for node labels internally offering the configuration option minDomains (See https://kubernetes.io/docs/concepts/scheduling-eviction/topology-spread-constraints/#spread-constraint-definition):

minDomains indicates a minimum number of eligible domains. This field is optional. A domain is a particular instance of a topology. An eligible domain is a domain whose nodes match the node selector.

WDYT?

Interestintg find. Will give it a look and see how that goes.
Btw, should we be worried about this: minDomains field is a beta field and disabled by default in 1.25.?

@@ -821,6 +821,7 @@ type ReplicationSpec struct {
Factor int32 `json:"factor,omitempty"`

// Zones defines an array of ZoneSpec that the scheduler will try to satisfy.
// IMPORTANT: Make sure that the replication factor defined is less than the number of available zones.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it strictly required to be "less" or can it be also "less or equal"

Copy link
Contributor

@JStickler JStickler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Docs team] Approving, since it looks like this is just adding the validation and the API is documented by #9315.

Copy link
Collaborator

@periklis periklis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, let's update the branch to keep in sync with generated files.

@periklis periklis merged commit df77bf6 into grafana:main May 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.